Codecov Report

❌ Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.32%. Comparing base (42c13b0) to head (cb87663).
⚠️ Report is 8 commits behind head on main.

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
+ Coverage   79.93%   80.32%   +0.39%     
==========================================
  Files         120      121       +1     
  Lines        4017     4031      +14     
  Branches      872      875       +3     
==========================================
+ Hits         3211     3238      +27     
+ Misses        618      616       -2     
+ Partials      188      177      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review

Thanks for this contribution! Adding netstandard2.0 support alongside net8.0 is a significant effort—the polyfills, #if guards, and nullability annotation fixes are well-structured overall. I found one bug and one minor dead code issue:

Critical: Wrong preprocessor symbol in ReflectionHelper.cs

File: src/CommandLineUtils/Internal/ReflectionHelper.cs (lines 196, 205 in the current file)
Severity: Critical

The #if directives use NET_6_0_OR_GREATER (with an underscore between NET and 6), but the correct .NET SDK-defined symbol is NET6_0_OR_GREATER (no underscore). Every other file in this PR uses the correct symbol.

Because NET_6_0_OR_GREATER is never defined, both the Equals and GetHashCode methods will always take the #else branch—even when targeting net8.0. On net8.0 this means:

• Equals falls back to MetadataToken/Module comparison instead of HasSameMetadataDefinitionAs (functionally equivalent in most cases but diverges from intent).
• GetHashCode uses the raw MetadataToken with the nil-token guard instead of HasMetadataToken()/GetMetadataToken(). The HasMetadataToken() API handles additional edge cases that the manual check may not cover.

Fix: Change NET_6_0_OR_GREATER → NET6_0_OR_GREATER on both lines.

Medium: Unused $testArgs variable in build.ps1

File: build.ps1 (lines 32-35)
Severity: Medium

The $testArgs array is constructed with --no-build, --configuration, and conditionally --collect, but it is never referenced. The actual dotnet test invocations on lines 43-46 hardcode the arguments. With Set-StrictMode -Version 1 this won't error (strict mode v1 doesn't flag unused variables), but it's dead code that could confuse future maintainers.

Suggested fix: Either remove the $testArgs construction, or use @testArgs splatting in the dotnet test call:

exec dotnet test @testArgs

Note: Test assertion style for null ShortName

File: test/CommandLineUtils.Tests/OptionAttributeTests.cs (lines 74, 173)
Severity: Low (not blocking)

Assert.Empty(o.ShortName ?? "test") will pass when ShortName is "" (correct) but will also silently pass when ShortName is unexpectedly null (producing Assert.Empty("test") which fails—so this is actually safe). Just noting the pattern is a bit non-obvious. No action needed.

Everything else in the PR looks correct. The StringExtensions wrapper with [NotNullWhen(false)] is a clean solution for the netstandard2.0 nullability gap, and the #if NET6_0_OR_GREATER guards on the trimming attributes are applied consistently.

🤖 Generated with Claude Code

Code Review

Thanks for this contribution! Adding .NET Framework (net472) support is a significant undertaking and the approach is well-structured. One issue found:

Critical: Wrong preprocessor symbol in ReflectionHelper.cs

File: src/CommandLineUtils/Internal/ReflectionHelper.cs, lines 196 and 205
Severity: Critical

The #if directives use NET_6_0_OR_GREATER (with an underscore between NET and 6), but the correct .NET SDK-defined symbol is NET6_0_OR_GREATER (no underscore between NET and 6). Every other file in this PR uses the correct form.

Because NET_6_0_OR_GREATER is never defined by the SDK, the #if branch is dead code — the #else fallback (manual MetadataToken comparison) will always be used, even on .NET 8+. While the fallback implementation is functionally correct and unlikely to cause runtime failures, it bypasses the framework-provided HasSameMetadataDefinitionAs and HasMetadataToken/GetMetadataToken APIs on platforms where they're available.

Suggested fix:

-#if NET_6_0_OR_GREATER
+#if NET6_0_OR_GREATER

in both locations (lines 196 and 205).

Everything else — the StringExtensions / DictionaryExtensions polyfills, PolySharp usage, #if NET6_0_OR_GREATER guards on trim attributes, nullable annotation fixes in tests, and the build script changes for skipping full framework tests on non-Windows — looks correct.

🤖 Generated with Claude Code

Code Review

Thanks for this contribution! Adding .NET Framework (net472) support is a significant effort involving many files. Most of the changes look correct — the polyfill strategy with PolySharp, the StringExtensions wrapper for nullability, DictionaryExtensions.TryAdd backfill, and the conditional #if guards on trimming attributes are all sound approaches.

I found one bug and one test concern:

1. Bug: Wrong preprocessor symbol in ReflectionHelper.cs

• File: src/CommandLineUtils/Internal/ReflectionHelper.cs, lines 196 and 205
• Severity: Critical
• Issue: The preprocessor directive uses NET_6_0_OR_GREATER (with underscores around the version) instead of the correct NET6_0_OR_GREATER.
• Why it matters: NET_6_0_OR_GREATER is not a defined symbol. The #if block will never be true, even on .NET 8+. This means on all target frameworks, the code will fall through to the #else branch — the manual MetadataToken comparison. While that fallback is functionally equivalent and won't cause runtime failures, it means HasSameMetadataDefinitionAs() and HasMetadataToken()/GetMetadataToken() are dead code, and .NET 8+ builds aren't using the intended API.
• Suggested fix: Change both occurrences to NET6_0_OR_GREATER (no underscore before 6), consistent with every other usage in this PR.

2. Test concern: Assert.Empty(o.ShortName ?? "test") weakens the assertion

• File: test/CommandLineUtils.Tests/OptionAttributeTests.cs, lines 74 and 121
• Severity: Medium
• Issue: The null-coalescing fallback ?? "test" means that if ShortName is unexpectedly null, the assertion silently passes (it asserts "test" is empty, which fails — but only by coincidence of the fallback value). More importantly, if someone changes the fallback string to "", the test would silently pass on null. The original test was asserting that ShortName is the empty string, not null.
• Why it matters: The test intent is to verify that short names are explicitly empty strings. A null value would indicate different behavior and should fail with a clear message rather than relying on an arbitrary fallback.
• Suggested fix: Use Assert.NotNull(o.ShortName) followed by Assert.Empty(o.ShortName), or simply Assert.Equal("", o.ShortName).

Everything else — the build script changes for non-Windows TFM filtering, the .runsettings workaround, the nullable annotation adjustments in tests, and the #if guards on [DynamicallyAccessedMembers] and [RequiresUnreferencedCode] — looks correct.

Code Review

Thanks for this contribution — adding .NET Framework (net472) multi-targeting is a substantial amount of work, and the approach is clean and well-structured.

I reviewed the full diff and found no substantive issues. A few observations:

Things that look correct

• #if NET6_0_OR_GREATER / #elif NET472_OR_GREATER / #else #error pattern: Consistently applied across all files that use .NET 6+ APIs (RequiresUnreferencedCode, DynamicallyAccessedMembers, HasSameMetadataDefinitionAs, etc.). The #error fallback is a nice safety net against future TFM additions that aren't handled.

• DictionaryExtensions.TryAdd polyfill (DictionaryExtensions.cs): Correctly targets IDictionary<TKey, TValue>, which Dictionary<TKey, TValue> implements. On .NET 6+ the built-in Dictionary.TryAdd method takes precedence over the extension method since instance methods are preferred over extension methods. The empty #if NET6_0_OR_GREATER block correctly compiles out the polyfill.

• ReflectionHelper.MethodMetadataEquality: The .NET Framework fallback (MetadataToken == MetadataToken && Module.Equals(Module)) correctly replicates the semantics of HasSameMetadataDefinitionAs. The GetHashCode nil-token check (using rowMask = 0x00FFFFFF) faithfully mirrors the runtime's own implementation.

• PolySharp + NullabilityHelpers.cs: PolySharp is added only for .NETFramework targets and generates internal polyfills. The existing NullabilityHelpers.cs is guarded by #if NETSTANDARD2_0 || NET46_OR_GREATER, which covers net472. Since PolySharp's source generator detects existing type definitions and skips them, there's no duplicate type risk.

• Nullable downgrade to annotations for net472: Appropriate since .NET Framework's compiler support for nullable warnings is limited.

• Build script: Correctly skips TestFullFramework on non-Windows (where Mono/.NET Framework test hosts aren't available).

• Test changes: The nullable annotation fixes (string?, Assert.IsAssignableFrom<PropertyInfo>(...), result?.ErrorMessage, etc.) are all mechanical adjustments to satisfy the stricter nullable analysis and are faithful to the original test intent.

No issues found — this looks good to merge.

Reviewed with Claude Code

Code Review

Thanks for this contribution! Restoring .NET Framework support is a valuable change for users who still target net472. The approach is well-structured — using #if/#elif/#else/#error guards consistently across the codebase, adding PolySharp for polyfills, and providing framework-appropriate alternatives for APIs like HasSameMetadataDefinitionAs and HasMetadataToken.

No substantive issues found

The changes look correct. A few notes on things I verified:

• ReflectionHelper.MethodMetadataEquality: The Equals/GetHashCode contract is maintained — Equals checks both MetadataToken and Module, while GetHashCode uses only MetadataToken. Two equal objects will always produce the same hash code.
• DictionaryExtensions.TryAdd: Correctly polyfills only for net472. On .NET 6+, the built-in Dictionary<,>.TryAdd instance method takes precedence and the extension isn't even compiled.
• NullabilityHelpers.cs: The existing #if NET46_OR_GREATER guard correctly activates for net472, providing NotNullWhenAttribute and AllowNullAttribute polyfills. PolySharp should handle the remaining trimming/AOT attributes (RequiresUnreferencedCode, DynamicallyAccessedMembers) that are conditionally excluded via #if NET6_0_OR_GREATER.
• Nullable downgrade to annotations for net472 in Directory.Build.props is the right call since .NET Framework doesn't have full nullable flow analysis support.
• Build script: The TestFullFramework=false flag for non-Windows correctly avoids running net472 tests on Linux/macOS where Mono may not be available.
• Test changes: The nullable annotation fixes (?., !, Assert.IsAssignableFrom<T> wrappers) are all mechanical adjustments needed for <Nullable>annotations</Nullable> mode and look correct.

🟢 Ship it.

Code Review

Thanks for this contribution! Restoring .NET Framework support is a solid improvement for the library's reach.

I reviewed the full diff — the multi-targeting approach with #if NET6_0_OR_GREATER / #elif NET472_OR_GREATER / #else #error guards is clean and provides a good safety net against future target framework changes slipping through undetected. The polyfill patterns (DictionaryExtensions.TryAdd, ReflectionHelper metadata token handling, PolySharp for language feature backfills) are all correctly implemented.

No substantive issues found. A few notes on specific areas I looked at closely:

• ReflectionHelper.MethodMetadataEquality: The GetHashCode implementation for net472 correctly uses only MetadataToken (without Module), which satisfies the Equals/GetHashCode contract — equal objects (same token AND module) will always produce the same hash. The extra collisions for cross-module cases are acceptable.
• DictionaryExtensions.TryAdd: Correctly targets IDictionary<TKey, TValue> as an extension method, and on .NET 6+ the empty #if block means Dictionary<T,V>.TryAdd (the instance method) is naturally preferred. Clean polyfill pattern.
• Test nullability fixes: The changes from direct casts to Assert.IsAssignableFrom<T>() and additions of ?. / ! operators are appropriate for stricter nullable analysis under net472's annotations mode.
• build.ps1 conditional: Skipping full-framework tests on non-Windows with -p:TestFullFramework=false is the right approach since net472 tests require Mono/Windows.
• params string?[] args → params string[]? args in CustomValidationAttributePasses: Correct — the [InlineData(null)] case passes a null array, not a null element.

Looks good to ship.

🤖 Generated with Claude Code

Code Review: Prepare 5.0.1 release

Thanks for this contribution! This is a well-structured release that restores .NET Framework 4.7.2 target framework support alongside .NET 8.0.

I reviewed all the substantive changes:

• Multi-targeting setup (net8.0;net472 in csproj files, PolySharp reference, Nullable downgrade to annotations for net472) — all looks correct.
• Conditional compilation guards (#if NET6_0_OR_GREATER / #elif NET472_OR_GREATER / #else #error) — consistent pattern applied throughout, with appropriate #error fallbacks to catch misconfigurations. The Pager.cs change correctly sets UseShellExecute = false only for .NET Framework where the default is true (it defaults to false on .NET Core+).
• DictionaryExtensions.TryAdd polyfill — correctly targets IDictionary<TKey, TValue> for .NET 4.7.2 only, and calling code uses the concrete Dictionary<TKey, TValue> type which has the built-in method on .NET 6+.
• ReflectionHelper.MethodMetadataEquality — the .NET Framework fallback using MetadataToken + Module comparison is the standard equivalent of HasSameMetadataDefinitionAs. The GetHashCode nil-token check mirrors the .NET runtime implementation. Safe for the reflected model types used here.
• Removal of NullabilityHelpers.cs — replaced by PolySharp, which provides these polyfills automatically.
• Test changes — nullable annotation fixes (? additions, ! null-forgiving operators), Assert.IsAssignableFrom<PropertyInfo> wrapping to satisfy nullability, and params string[]? adjustment are all mechanical adaptations for stricter nullable analysis. No test logic changes.
• Build script — TestFullFramework=false flag for non-Windows CI is the right approach since .NET Framework tests can't run on Linux.

No substantive issues found. The changes look correct.